Skip to content

Consistency fix (breaking): LibraryItemKey is replaced by CollectionKey and ContainerKey#379

Merged
bradenmacdonald merged 2 commits intoopenedx:masterfrom
open-craft:braden/base-key-refactor
Apr 23, 2025
Merged

Consistency fix (breaking): LibraryItemKey is replaced by CollectionKey and ContainerKey#379
bradenmacdonald merged 2 commits intoopenedx:masterfrom
open-craft:braden/base-key-refactor

Conversation

@bradenmacdonald
Copy link
Contributor

@bradenmacdonald bradenmacdonald commented Apr 23, 2025 •
edited
Loading

The opaque keys used for items in libraries are:

  • LibraryUsageLocatorV2 ( a UsageKey - the superclass usagekey is for XBlocks anywhere). Well established.
  • LibraryCollectionLocator (a LibraryItemKey - a new superclass for non-XBlock items in libraries). New-ish.
  • LibraryContainerLocator (a LibraryItemKey - a new superclass for non-XBlock items in libraries). New.

Thinking about this, it's going to be very confusing that we use the term "item" for anything in a library but XBlock keys in libraries (LibraryUsageLocatorV2) are not LibraryItemKeys. It's also weird that the base UsageKey is for XBlocks anywhere but the base LibraryItemKey is for two different types of things and is specific to libraries.

This PR clears up this and makes it consistent via a slight breaking change to the following:

  • LibraryUsageLocatorV2 ( a UsageKey ) - no change
  • LibraryCollectionLocator (change to a CollectionKey , a new base class)
  • LibraryContainerLocator (change to a ContainerKey, a new base class for containers in libraries and - in the future - in courses)
  • (Future: also we'd have a LibraryAssetKey which subclasses the existing AssetKey for static assets in libraries).

This is a breaking change but won't affect much code and I think it's important to avoid confusion and mess in the future. Here are the corresponding changes to content_libraries and related apps in edx-platform: openedx/openedx-platform#36588

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 23, 2025
@openedx-webhooks
Copy link

openedx-webhooks commented Apr 23, 2025

Thanks for the pull request, @bradenmacdonald!

This repository is currently maintained by @openedx/axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@github-project-automation github-project-automation bot moved this to Needs Triage in Contributions Apr 23, 2025
bradenmacdonald
bradenmacdonald commented Apr 23, 2025
View reviewed changes
opaque_keys/edx/keys.py
Comment on lines -14 to -15
if TYPE_CHECKING:
from opaque_keys.edx.locator import LibraryLocatorV2
Copy link
Contributor Author

@bradenmacdonald bradenmacdonald Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor allows us to get rid of this messy import of a locator (subclass) in the keys (parent class) file.

bradenmacdonald added a commit to open-craft/openedx-platform that referenced this pull request Apr 23, 2025
@bradenmacdonald
chore: update content libraries opaque key usages for opaque-keys v3
f35d60a 
See openedx/opaque-keys#379
@bradenmacdonald bradenmacdonald force-pushed the braden/base-key-refactor branch from 8f95269 to 300d036 Compare April 23, 2025 20:01
@bradenmacdonald bradenmacdonald mentioned this pull request Apr 23, 2025
Merged
@bradenmacdonald bradenmacdonald force-pushed the braden/base-key-refactor branch from 300d036 to f4f227f Compare April 23, 2025 20:05
@bradenmacdonald bradenmacdonald mentioned this pull request Apr 23, 2025
Closed
@bradenmacdonald bradenmacdonald force-pushed the braden/base-key-refactor branch from f4f227f to 8574864 Compare April 23, 2025 20:15
@codecov
Copy link

codecov bot commented Apr 23, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.00%. Comparing base (15ae68c) to head (ce976fb).
Report is 17 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
+ Coverage   93.84%   94.00%   +0.15%     
==========================================
  Files          31       31              
  Lines        3007     3036      +29     
  Branches      192      191       -1     
==========================================
+ Hits         2822     2854      +32     
+ Misses        159      156       -3     
  Partials       26       26
Flag Coverage Δ
unittests 94.00% <100.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bradenmacdonald bradenmacdonald merged commit ee11894 into openedx:master Apr 23, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in Contributions Apr 23, 2025
@bradenmacdonald bradenmacdonald deleted the braden/base-key-refactor branch April 23, 2025 21:40
bradenmacdonald added a commit to openedx/openedx-platform that referenced this pull request Apr 23, 2025
@bradenmacdonald
chore: bump opaque-keys to v3, update content libraries key usages (#…
867e246 
…36588)

See openedx/opaque-keys#379
@navinkarkera navinkarkera mentioned this pull request Apr 24, 2025
Closed
UsamaSadiq pushed a commit to openedx/openedx-platform that referenced this pull request May 14, 2025
@bradenmacdonald @UsamaSadiq
chore: bump opaque-keys to v3, update content libraries key usages (#…
9192c07 
…36588)

See openedx/opaque-keys#379
marlonkeating pushed a commit to openedx/openedx-platform that referenced this pull request Jul 15, 2025
@bradenmacdonald @marlonkeating
chore: bump opaque-keys to v3, update content libraries key usages (#…
df431c3 
…36588)

See openedx/opaque-keys#379
@openedx-webhooks openedx-webhooks added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ormsbee ormsbee ormsbee approved these changes

Assignees

No one assigned

Labels

core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bradenmacdonald @openedx-webhooks @ormsbee